v2v/tests/src/nbdkit/nbdkit.py (3)

763-778: ⚠️ Potential issue | 🟠 Major

Test does not verify zero accounting as stated in PR objectives.

The PR objective mentions verifying "read, write, zeroed, trimmed" but the qemu_cmd only issues write, read, and discard operations. The expected_patterns list lacks a pzero assertion, so a regression in zero handling would still pass.

Suggested fix to add zero operation

-            qemu_cmd = f"qemu-io -f raw nbd://localhost -c \'write 0 1M\' -c \'read 0 1M\' -c \'discard 0 1M\'"
+            qemu_cmd = "qemu-io -f raw nbd://localhost -c 'write 0 1M' -c 'read 0 1M' -c 'write -z 1M 1M' -c 'discard 0 1M'"

             result = process.run(qemu_cmd, shell=True)
             if result.exit_status != 0:
-                test.fail(f"qemu-io failed")
+                test.fail("qemu-io failed")

             # VALIDATION: Check the log file for the read, written, zeroed, trimmed  reports
             with open(log_path, 'r') as f:
                 log_content = f.read()
-                LOG.info(f"logging info ----> {log_content}")
+                LOG.info("logging info ----> %s", log_content)
                 # Look for the expected pattern
-                expected_patterns = ["count: pwrite count=1048576", "count: pread count=1048576", "count: trim count=1048576"]
+                expected_patterns = [
+                    "count: pwrite count=1048576",
+                    "count: pread count=1048576",
+                    "count: pzero count=1048576",
+                    "count: trim count=1048576",
+                ]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 763 - 778, The test claims to
verify read, write, zeroed and trimmed but only issues write/read/discard;
update the qemu_cmd string (variable qemu_cmd) to include a zero operation
(e.g., add a -c 'zero 0 1M' clause) and add the corresponding zero accounting
assertion to expected_patterns (e.g., include "count: pzero count=1048576");
ensure you still read log_path and call test.fail if the new pzero pattern is
missing.

---

747-759: ⚠️ Potential issue | 🟠 Major

Process handle p uninitialized before try; startup readiness still relies on fixed sleep.

If Popen raises an exception before assigning to p, the finally block at line 783 will fail with NameError. Additionally, the fixed 1-second sleep is inherently racey and will cause flakiness on slower hosts.

Suggested fix

+        p = None
         try:
             with open(log_path, "w") as log_file:
-                LOG.info(f"Starting nbdkit")
+                LOG.info("Starting nbdkit")
                 cmd = ["nbdkit", "-f", "-v", "memory", "10M", "--filter=count"]
-                # Start nbdkit as a background process, piping stderr to log file
                 p = Popen(cmd, stderr=log_file, stdout=log_file)

-            if p.poll() is not None:
-                test.fail(f"nbdkit failed to start! Check {log_path}")
-
-            # Allow nbdkit instances time to start listening
             import time
-            time.sleep(1)
+            deadline = time.time() + 10
+            while time.time() < deadline:
+                if p.poll() is not None:
+                    test.fail("nbdkit exited before becoming ready; check %s" % log_path)
+                probe = process.run("nbdinfo nbd://localhost", shell=True, ignore_status=True)
+                if probe.exit_status == 0:
+                    break
+                time.sleep(0.5)
+            else:
+                test.fail("nbdkit failed to become ready; check %s" % log_path)

Also update the finally block to guard against p being None:

         finally:
             LOG.info("Cleanup: Kill the processes")
-            if p.poll() is None:
+            if p is not None and p.poll() is None:
                 p.terminate()

---

749-749: ⚠️ Potential issue | 🟡 Minor

Remove extraneous f prefix from string literals without placeholders.

Static analysis flagged f-strings without placeholders at lines 749 and 767. Use plain strings instead.

-                LOG.info(f"Starting nbdkit")
+                LOG.info("Starting nbdkit")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@v2v/tests/src/nbdkit/nbdkit.py` at line 749, Replace unnecessary f-string
literals passed to LOG.info in nbdkit.py with plain string literals: locate the
LOG.info(...) calls that use an f-string but contain no placeholders (e.g.,
LOG.info(f"Starting nbdkit")) and remove the leading f so they become normal
strings; ensure any other similar f"..." usages in the same module that have no
interpolation are changed the same way to satisfy static analysis.

v2v/tests/src/nbdkit/nbdkit.py (1)

817-829: ⚠️ Potential issue | 🟠 Major

Initialize p before try and replace fixed sleep with readiness polling.

At Line 822, p is only assigned inside try; if Popen fails, Line 857 can raise on undefined p and hide the startup error. Also, Line 829 uses a fixed sleep, which is race-prone.

Suggested hardening

     def test_count_filter():
         from subprocess import Popen, TimeoutExpired

         log_path = os.path.join(data_dir.get_tmp_dir(), "nbdkit_count_filter_debug.log")

+        p = None
         try:
             with open(log_path, "w") as log_file:
                 LOG.info("Starting nbdkit")
                 cmd = ["nbdkit", "-f", "-v", "memory", "10M", "--filter=count"]
                 # Start nbdkit as a background process, piping stderr and stdout to log file
                 p = Popen(cmd, stderr=log_file, stdout=log_file)

             if p.poll() is not None:
                 test.fail(f"nbdkit failed to start! Check {log_path}")

-            # Allow nbdkit instances time to start listening
+            # Wait for nbdkit to become reachable
             import time
-            time.sleep(1)
+            deadline = time.time() + 10
+            while time.time() < deadline:
+                if p.poll() is not None:
+                    test.fail(f"nbdkit exited before becoming ready! Check {log_path}")
+                probe = process.run("nbdinfo nbd://localhost", shell=True, ignore_status=True)
+                if probe.exit_status == 0:
+                    break
+                time.sleep(0.5)
+            else:
+                test.fail(f"nbdkit did not become ready in time! Check {log_path}")
@@
         finally:
             # Cleanup: Kill the processes
             LOG.info("Cleanup: Kill the processes")
-            if p.poll() is None:
+            if p is not None and p.poll() is None:
                 p.terminate()
                 try:
                     # Wait up to 5 seconds for it to exit
                     p.wait(timeout=5)

Also applies to: 854-858

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 817 - 829, The code assigns
Popen to p only inside the try block and then uses p later, which can raise
UnboundLocalError if Popen fails, and it uses a fixed time.sleep for startup
which is racy; initialize p = None before the try, wrap Popen in try/except to
log and fail early if it raises, and replace the fixed time.sleep with a
readiness polling loop that (within a timeout) repeatedly checks p.poll() for
immediate failure and attempts to verify nbdkit is listening (e.g., try
connecting to the expected socket/port or scanning the log file for a
"listening" message) until success or timeout; update the same pattern for the
other block mentioned (lines 854-858) and reference Popen, p, and log_path when
making the changes.

v2v/tests/src/nbdkit/nbdkit.py (1)

827-845: ⚠️ Potential issue | 🟠 Major

Harden readiness + log-capture timing to avoid flaky assertions.

On Line 828/Line 833, PID+sleep(1) is racey; on Line 844, reading output before stopping can miss buffered count: lines. This can intermittently fail/pass depending on host timing.

Suggested fix

         p = None
         try:
@@
-            # Check if the process actually started (has a PID)
+            # Check if the process actually started (has a PID)
             if not p.get_pid():
                 test.fail("nbdkit failed to start!")
 
-            # Allow nbdkit instance time to start listening
-            import time
-            time.sleep(1)
+            # Wait until nbdkit is actually reachable
+            import time
+            deadline = time.time() + 10
+            while time.time() < deadline:
+                probe = process.run("nbdinfo nbd://localhost", shell=True, ignore_status=True)
+                if probe.exit_status == 0:
+                    break
+                if p.poll() is not None:
+                    test.fail("nbdkit exited before becoming ready")
+                time.sleep(0.5)
+            else:
+                test.fail("nbdkit did not become ready within timeout")
@@
-            # VALIDATION: Check the captured output
-            # SubProcess captures stdout and stderr separately as bytes
-            log_content = (p.get_stdout() + p.get_stderr()).decode('utf-8', errors='replace')
-            LOG.info(f"Captured output: {log_content}")
+            # Stop first so output is flushed before validation
+            if p and p.poll() is None:
+                p.stop(timeout=5)
+            log_content = (p.get_stdout() + p.get_stderr()).decode("utf-8", errors="replace")
+            LOG.info("Captured output: %s", log_content)
@@
         finally:
             # Cleanup using the class methods
             if p and p.poll() is None:
                 LOG.info("Cleanup: Stopping nbdkit process")
-                # stop() is better than terminate() here as it handles the wait() logic
                 p.stop(timeout=5)

Also applies to: 858-864

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 827 - 845, The start/readiness
and log-capture are racy: replace the PID check + time.sleep(1) around
p.get_pid()/time.sleep with an active readiness probe (e.g., poll the nbd
socket/port or loop-read p.get_stdout() for a startup log line) to wait until
nbdkit is actually listening before running qemu-io, and move the
captured-output read to after the nbdkit instance is cleanly stopped/terminated
(use p.stop() or p.wait() and then read p.get_stdout() + p.get_stderr()) so
buffered "count:" lines are not missed; update references in this block to
p.get_pid(), time.sleep, process.run (qemu_cmd) and
p.get_stdout()/p.get_stderr() accordingly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes